-
Notifications
You must be signed in to change notification settings - Fork 1.1k
statement-store: New RPC result types #10421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Wayyyy better than before, thank you for the effort! 😉 |
carlosala
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
|
Probably the next step is to rethink the whole API, reducing the surface and providing clear endpoints 👍🏻 |
|
I looked at all cases where we use SubmitResult (internal, not RPC types). I think we could be more user-friendly by showing what exactly happened and helping users decide how to fix the problem. Then we can adjust RPC types according to that. Current SubmitResult
Proposed SubmitResult |
|
/cmd prdoc --audience node_dev --bump major |
| pub const GOOD_STATEMENT: Rep = Rep::new(1 << 7, "Good statement"); | ||
| /// Reputation change when a peer sends us a bad statement. | ||
| pub const BAD_STATEMENT: Rep = Rep::new(-(1 << 12), "Bad statement"); | ||
| pub const GOOD_STATEMENT: Rep = Rep::new(1 << 8, "Good statement"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean up. We never used the old GOOD_STATEMENT, so I removed it. But since we have only EXCELENT_STATEMENT, no need to keep it excelent, just good is ok
| use std::sync::Arc; | ||
|
|
||
| /// Maps the internal InvalidReason to the RPC API InvalidReason type. | ||
| fn map_invalid_reason(reason: sp_statement_store::InvalidReason) -> InvalidReason { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: they seem to have the same invariant, so probably can be a From implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't add From here, either I didn't want to bring extra crates to dependencies.
Co-authored-by: Alexandru Gheorghe <[email protected]>
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
| Low, | ||
| /// Reason why a statement was rejected from the store. | ||
| #[derive(Debug, Eq, PartialEq)] | ||
| pub enum RejectionReason { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have you duplicated these types? Because of serde? Make it an optional dependency.
Description
Internal submittion result type changed to keep more information, which should be useful for RPC clients.
Application-level errors moved from JSON-RPC transport errors into the result type for
statement_submitRPC method to help clients distinguish between submission outcomes instead of parsing error strings.Runtime API wasn't changed.
Integration
Breaking Change -
statement_submitnow returnsStatementSubmitResultenum instead of().Review Notes
Submission outcomes now return as Ok(result) with status field. Only infrastructure failures (database errors, decode errors) remain as JSON-RPC errors.
NetworkPriority removed as we never used it, reputation changes adjusted.